-
Notifications
You must be signed in to change notification settings - Fork 98
feat/idle-channel-eviction #2651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Change-Id: I62fe152c293438bf64b657b5b1fe795e22ce9c85
Change-Id: I8d4212aad0ca7613b6c33d69d925671a090d1609
Change-Id: I9ad5eeaacb02ace9ba3cf7c09b6846dcbc298fb8
Change-Id: I04698ba82b95c82301e4e9f436401698675d7ea9
…googleapis#2649) * chore: Update generation configuration at Thu Jul 31 02:47:07 UTC 2025 * chore: Update generation configuration at Fri Aug 1 02:54:57 UTC 2025 * chore: Update generation configuration at Sat Aug 2 02:42:43 UTC 2025 * chore: generate libraries at Sat Aug 2 02:43:12 UTC 2025 * chore: Update generation configuration at Tue Aug 5 02:50:25 UTC 2025 * chore: generate libraries at Tue Aug 5 02:50:52 UTC 2025 * chore: Update generation configuration at Wed Aug 6 02:48:20 UTC 2025 * chore: generate libraries at Wed Aug 6 02:48:53 UTC 2025 * chore: Update generation configuration at Thu Aug 7 02:49:23 UTC 2025
* chore(main): release 2.64.0 * chore: generate libraries at Fri Aug 8 14:28:59 UTC 2025 --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: cloud-java-bot <[email protected]>
…e is slow (1m, 2 channel) (googleapis#2656) …conservative Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [ ] Make sure to open an issue as a [bug/issue](https://togithub.com/googleapis/java-bigtable/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [ ] Ensure the tests and linter pass - [ ] Code coverage does not decrease (if any source code was changed) - [ ] Appropriate docs were updated (if necessary) - [ ] Rollback plan is reviewed and LGTMed - [ ] All new data plane features have a completed end to end testing plan Fixes #<issue_number_goes_here> ☕️ If you write sample code, please follow the [samples format]( https://togithub.com/GoogleCloudPlatform/java-docs-samples/blob/main/SAMPLE_FORMAT.md).
🤖 I have created a release *beep* *boop* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
* chore(main): release 2.65.0 * chore: generate libraries at Tue Aug 12 16:25:49 UTC 2025 --------- Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: cloud-java-bot <[email protected]>
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Warning: This pull request is touching the following templated files:
|
Change-Id: I3baa8e93bc614efe21f1159a708be36881120e34
Change-Id: I11e8da901a93a49009caf4caf0385fd5e3c8091b
@@ -31,4 +33,11 @@ private NoOpChannelPrimer() {} | |||
public void primeChannel(ManagedChannel managedChannel) { | |||
// No op | |||
} | |||
|
|||
@Override | |||
public SettableApiFuture<PingAndWarmResponse> sendPrimeRequestsAsync(ManagedChannel var1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
public SettableApiFuture<PingAndWarmResponse> sendPrimeRequestsAsync(ManagedChannel var1) { | |
public SettableApiFuture<PingAndWarmResponse> sendPrimeRequestsAsync(ManagedChannel channel) { |
import javax.annotation.Nullable; | ||
|
||
/** Class that manages the health checking in the BigtableChannelPool */ | ||
public class ChannelPoolHealthChecker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be package private?
final ConcurrentLinkedQueue<ProbeResult> probeHistory = new ConcurrentLinkedQueue<>(); | ||
|
||
// we keep both so that we don't have to check size() on the ConcurrentLinkedQueue all the time | ||
AtomicInteger failedProbesInWindow = new AtomicInteger(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final?
// Window_Duration is the duration over which we keep probe results | ||
private static final Duration WINDOW_DURATION = Duration.ofMinutes(5); | ||
// Interval at which we probe channel health | ||
static final Duration PROBE_INTERVAL = Duration.ofSeconds(30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where else is this used? can it be private? or tagged with @visibileForTesting
?
BigtableChannelPrimer primer = (BigtableChannelPrimer) channelPrimer; | ||
probeFuture = primer.sendPrimeRequestsAsync(entry.getManagedChannel()); | ||
} else if (channelPrimer instanceof NoOpChannelPrimer) { | ||
NoOpChannelPrimer primer = (NoOpChannelPrimer) channelPrimer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to probe if the channel primer is noop channel primer? sendPrimerRequestAsync doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm probably not. We can just have it fall into the continue block.
...le-cloud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/grpc/BigtableChannelPool.java
Show resolved
Hide resolved
...oud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/grpc/ChannelPoolHealthChecker.java
Outdated
Show resolved
Hide resolved
/** | ||
* Finds a channel that is an outlier in terms of health. | ||
* | ||
* @return Entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
* @return Entry | |
* @return the entry to be evicted. Returns null if nothing to evict. |
executor.scheduleAtFixedRate( | ||
this::detectAndRemoveOutlierEntries, | ||
initialDelayDetect.toMillis(), | ||
PROBE_INTERVAL.toMillis(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can schedule this task every MIN_EVICTION_INTERVAL millis, and we won't need the check in detectAndRemoveOutlierEntries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that probably works and would avoid some thread calls. I think it would make the channel eviction slightly less responsive though? It might mean that if a channel becomes unresponsive at time T = 10:02 it waits ~ 9:58 to get evicted.
...oud-bigtable/src/main/java/com/google/cloud/bigtable/gaxx/grpc/ChannelPoolHealthChecker.java
Show resolved
Hide resolved
Change-Id: Ie0fde53cd17b1feeaaffd15382e936a46d84c5c1
Change-Id: I62fe152c293438bf64b657b5b1fe795e22ce9c85
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> ☕️
If you write sample code, please follow the samples format.